Replaced field_masking_span occurrences with respective ParseField #74718

GrayFox1 · 2021-06-29T19:24:06Z

Replaced field_masking_span occurrences with respective ParseField. This change improves maintainability, since some hard coded Strings were replaced by the ParseField that i've defined in the FieldMaskingSpanQueryBuilder Class, besides being consistent with other span queries.

I've ran the tests with "gradlew :server:test --tests org.elasticsearch.index.*" and i would appreciate if someone could give me some feedback.

mark-vieira · 2021-06-29T19:38:33Z

@elasticmachine retest this please

mark-vieira · 2021-06-29T20:35:41Z

@elasticmachine retest this please

mark-vieira · 2021-06-29T20:38:19Z

@elasticmachine test this pleasee

romseygeek

Thanks for opening this pull request @GrayFox1! I left a couple of comments but this looks pretty close to me.

romseygeek · 2021-06-30T08:20:40Z

server/src/main/java/org/elasticsearch/index/query/FieldMaskingSpanQueryBuilder.java

@@ -110,7 +110,7 @@ public static FieldMaskingSpanQueryBuilder fromXContent(XContentParser parser) t
                        throw new ParsingException(parser.getTokenLocation(), "[field_masking_span] query must be of type span query");


Can we replace the name in these error messages as well?

Sure, i'll fix those messages in my next commit

romseygeek · 2021-06-30T08:22:05Z

server/src/test/java/org/elasticsearch/index/query/FieldMaskingSpanQueryBuilderTests.java

@@ -73,13 +74,13 @@ public void testFromJson() throws IOException {
                "}";
        Exception exception = expectThrows(ParsingException.class, () -> parseQuery(json));
        assertThat(exception.getMessage(),
-            equalTo("field_masking_span [query] as a nested span clause can't have non-default boost value [0.23]"));
+            equalTo(NAME.getPreferredName() + " [query] as a nested span clause can't have non-default boost value [0.23]"));
    }



Can we also add a test that uses the old deprecated name, and checks for a deprecation warning? There's an assertWarnings() helper method that will check for specific deprecation strings.

Sure, i'll add a new test in my next commit.

romseygeek · 2021-06-30T08:30:57Z

@elasticmachine test this please

romseygeek · 2021-06-30T10:02:29Z

There's a failure in SearchModuleTests which I think can be resolved by adding span_field_masking to a list of expected queries and field_masking_span to a list of expected deprecated queries.

GrayFox1 · 2021-06-30T22:58:30Z

Thanks for your feedback @romseygeek! I just updated my pull request according to your comments and ran all tests with gradlew :server:test --tests org.elasticsearch.index.*

romseygeek

Thanks @GrayFox1, this looks great. Two more nits and then I think we're done, as long as CI is happy.

romseygeek · 2021-07-01T07:45:34Z

.idea/eclipseCodeFormatter.xml

@@ -11,4 +11,4 @@
      </ProjectSpecificProfile>
    </option>
  </component>
-</project>
+</project>


Nit: can you revert this?

Sure, i guess this happened when i was trying to configure the eclipseCodeFormatter plugin, but i'll revert this in my next commit.

romseygeek · 2021-07-01T07:47:37Z

server/src/test/java/org/elasticsearch/search/SearchModuleTests.java

@@ -303,7 +304,7 @@ public void testRegisterRescorer() {
            "combined_fields",
            "dis_max",
            "exists",
-            "field_masking_span",
+            NAME.getPreferredName(),


Let's just use "span_field_masking" here to keep in line with the other entries in the list.

romseygeek · 2021-07-01T07:48:39Z

@elasticmachine test this please

romseygeek · 2021-07-01T08:17:07Z

@GrayFox1 you'll need to merge in the latest version of our master branch to get tests to pass (we've had a version bump internally so all the BWC checks are complaining).

elasticmachine · 2021-07-01T18:46:56Z

Pinging @elastic/es-search (Team:Search)

GrayFox1 · 2021-07-01T21:16:22Z

Thanks @romseygeek! I just updated my pull request according to your comments, ran the tests with gradlew :server:test --tests org.elasticsearch.index* and all of them passed.

There is only one test failing when i ran gradlew :server:test --tests org.elasticsearch.search* in the SearchModuleTests. The failure is on the testRegisteredQueries when it iterates on the expected queries list and can't find "span_field_masking", although i have added this query in the list.

I tried looking into the SearchModule to check if the QuerySpec Registration wasn't using the ParseField, but everything looks ok.

romseygeek · 2021-07-05T09:24:44Z

@elasticmachine test this please

romseygeek · 2021-07-05T10:08:01Z

Aha, this looks like it's a bug in the test - it's filtering out queries that have any deprecated names, not queries that have been entirely deprecated. I'll open a separate fix for that.

romseygeek · 2021-07-05T11:23:31Z

I opened #74906 to fix the search test issue - you can merge that in here first as well if you like so that we can see how happy CI is otherwise.

romseygeek · 2021-07-08T15:18:04Z

@elasticmachine update branch

romseygeek · 2021-07-08T15:18:31Z

@elasticmachine ok to test

romseygeek · 2021-07-09T07:56:50Z

Thanks @GrayFox1!

elasticsearchmachine · 2021-07-09T07:59:15Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
backport --pr 74718

`field_masking_span` is the only span query that does not begin with `span_`. This commit deprecates the existing name and adds a new name `span_field_masking` to better fit with the other queries.

romseygeek · 2021-07-09T08:16:02Z

Manually backported in fc5b80a

GrayFox1 added 2 commits June 29, 2021 01:33

Replaced field_masking_span occurrences with respective ParseField

6d9935b

Replaced field_masking_span occurrences with respective

c01be61

elasticsearchmachine added v8.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jun 29, 2021

romseygeek reviewed Jun 30, 2021

View reviewed changes

GrayFox1 added 3 commits June 30, 2021 16:37

Updates error messages

1264af8

Updates list of expected deprecated queries

c9b9237

Adds the test testJsonWithDeprecatedName

68a5210

romseygeek suggested changes Jul 1, 2021

View reviewed changes

GrayFox1 added 5 commits July 1, 2021 10:49

Updates list of expected queries

c8048ee

Removes unused import and Reverts changes to eclipseCodeFormatter

08f92b6

Reverts changes to eclipseCodeFormatter

16c220b

Merge remote-tracking branch 'upstream/master'

e2a053f

Fixes expected queries lists alphabetical order

c6751ff

jtibshirani added the :Search/Search Search-related issues that do not fall into other categories label Jul 1, 2021

elasticmachine added the Team:Search Meta label for search team label Jul 1, 2021

GrayFox1 requested a review from romseygeek July 2, 2021 13:32

Merge branch 'master' into master

cc067db

romseygeek added auto-backport Automatically create backport pull requests when merged auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) v7.15.0 labels Jul 9, 2021

romseygeek merged commit bb37e09 into elastic:master Jul 9, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

joegallo added the >enhancement label Sep 22, 2021

		@@ -110,7 +110,7 @@ public static FieldMaskingSpanQueryBuilder fromXContent(XContentParser parser) t
		throw new ParsingException(parser.getTokenLocation(), "[field_masking_span] query must be of type span query");

Replaced field_masking_span occurrences with respective ParseField #74718

Replaced field_masking_span occurrences with respective ParseField #74718

Uh oh!

Conversation

GrayFox1 commented Jun 29, 2021

Uh oh!

mark-vieira commented Jun 29, 2021

Uh oh!

mark-vieira commented Jun 29, 2021

Uh oh!

mark-vieira commented Jun 29, 2021

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

romseygeek Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

GrayFox1 Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

romseygeek Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

GrayFox1 Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Jun 30, 2021

Uh oh!

romseygeek commented Jun 30, 2021

Uh oh!

GrayFox1 commented Jun 30, 2021

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

romseygeek Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

GrayFox1 Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

romseygeek Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Jul 1, 2021

Uh oh!

romseygeek commented Jul 1, 2021

Uh oh!

elasticmachine commented Jul 1, 2021

Uh oh!

GrayFox1 commented Jul 1, 2021

Uh oh!

romseygeek commented Jul 5, 2021

Uh oh!

romseygeek commented Jul 5, 2021

Uh oh!

romseygeek commented Jul 5, 2021

Uh oh!

romseygeek commented Jul 8, 2021

Uh oh!

romseygeek commented Jul 8, 2021

Uh oh!

romseygeek commented Jul 9, 2021

Uh oh!

elasticsearchmachine commented Jul 9, 2021

💔 Backport failed

Uh oh!

romseygeek commented Jul 9, 2021

Uh oh!

Uh oh!